-
Notifications
You must be signed in to change notification settings - Fork 233
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: add descriptions without modifying model and protocol digest #111
Conversation
src/uagents/models.py
Outdated
orig_descriptions[field_name] = {} | ||
Model._remove_descriptions(field.type_, orig_descriptions[field_name]) | ||
|
||
@staticmethod |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From my point of view these methods shouldn't be static most of the times the model is even passed in why not use self
then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The model param passed to these newly created methods is actually not an instance of a Model subclass but rather a Model subclass type (so a class object) that's why I couldn't implement these methods as instance methods (so using self.). However you are right in the sense that these method could be class methods using cls.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the refactored version I created class methods instead of static methods where possible, please check it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this feature needs to be rethought as it can produce different digests for the same model. Also removing model fields may not be the best approach to skip them in digest computation. One can simply delete the fields from the manifest dict.
src/uagents/protocol.py
Outdated
@@ -176,7 +176,7 @@ def manifest(self) -> Dict[str, Any]: | |||
|
|||
for schema_digest, model in all_models.items(): | |||
manifest["models"].append( | |||
{"digest": schema_digest, "schema": model.schema()} | |||
{"digest": schema_digest, "schema": model.schema_no_descriptions} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it guaranteed that this value won't be None?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the refactored version I call the schema_no_descriptions(), that method also uses a class attribute as a cache and that class attribute could possibly be None, but in the schema_no_descriptions() method I guarantee to set the value of that class variable now if that is None.
@@ -199,6 +199,12 @@ def manifest(self) -> Dict[str, Any]: | |||
encoded = json.dumps(manifest, indent=None, sort_keys=True).encode("utf8") | |||
metadata["digest"] = f"proto:{hashlib.sha256(encoded).digest().hex()}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
digest is also computed by the compute_digest
static method.
It takes as input the manifest which will contain the descriptions hence producing a different digest.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I modified this method accordingly and also created test for it in test_fields_descr.py, please check it.
Closing this because we will soon be revising the way the model digests are done anyway. |
No description provided.